Fix chip info in the ROM/ROM_EXT #28418
Draft
+162
−107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pamaury
force-pushed
the
fix_chip_info
branch
3 times, most recently
from
cb4e92a to
30d6790
Compare
engdoreis
reviewed
Oct 6, 2025
| ], | ||
| ) | ||
|
|
||
| opentitan_test( |
There was a problem hiding this comment.
Can you please add an entry to the tesplan?
pamaury
force-pushed
the
fix_chip_info
branch
from
30d6790 to
3a5ac06
Compare
Without this option, the chip_info symbol might get discarded if it is not used directly. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
fix_chip_info
branch
5 times, most recently
from
ee183a0 to
2e1b8b4
Compare
cfrantz
approved these changes
Oct 15, 2025
The name chip_info is misleading because the data is embedded in every binary. So it actually is a build info. The chip_info is simply then the ROM's build_info. Signed-off-by: Amaury Pouly <[email protected]>
The chip_info became broken without anyone realizing because it was untested. This test ensures that at least the chip info is filled and has the expected version. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
fix_chip_info
branch
from
2e1b8b4 to
30599ac
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WARNING: text below refers to the old version of the PR
While looking at the chip info, I realized that there were several problems. Originally, the issue comes from the fact that we have a single
cc_librarythat contains both thechip_info.hheader and thechip_info.cdata. This means that any binary using this dependency ends up including thekChipInfodata even if they didn't intend to. Coupled with the fact that several pieces of code directly refer tokChipInfoinstead of the linker symbol_chip_info_startmeans that for example part of the ROM_EXT doesn't actually read from the ROM's chip_info but from a ROM_EXT-embedded chip info!I decided to fix that by splitting the library into the header (which is what everyone but the ROM should use) and the data. I also eliminated the
kChipInfosymbol from the header entirely and replaced all users with_chip_info_start. While doing so, I stumbled on a subtle linker problem which is that sincekChipInfobecomes unused, it won't participate in linking (even if the section is marked as NODISARD) unless--whole-archiveis used which fortunaltely bazel has a setting for (alwayslink=True).I also decided to create a new ROM end-to-end test for the chip info to make sure that the data is present there. Maybe it might be worth also create a ROM_EXT end-to-end test to make sure that it uses the ROM chip info and not some other chip info but I leave that to a future PR.